test(selfupdate): pin the archive path guards, reject zero-length binaries, add fuzzing - #64
Open
aaearon wants to merge 6 commits into
Open
test(selfupdate): pin the archive path guards, reject zero-length binaries, add fuzzing#64aaearon wants to merge 6 commits into
aaearon wants to merge 6 commits into
Conversation
Close the archive-extraction findings from the mutation audit (SFU-01..09, SFU-20..22). Tests - buildTarGzEntries/buildZipEntries give full control over type flag, link name and declared size; buildTarGz/buildZip become thin wrappers. - TestExtractBinary switches to wantErrContains and puts a valid "grant" beside every hostile entry, so a rejection can never be attributed to the archive simply not containing a binary. - New TestCheckArchivePath pins one diagnostic per guard, including the previously missing empty-name, lowercase and forward-slash drive forms and the backslash traversal/UNC forms. - TestExtractBinaryRejectsNonRegularEntries covers symlink, hardlink and directory entries named "grant" in tar plus a zip directory entry. - Native fuzz targets for checkArchivePath and both extractors, seeded with the hostile cases; extended fuzzing stays out of CI. - TestExtractBinaryRejectsTruncatedEntry renamed to ...TruncatedArchive: it pins gzip-stream truncation, not the unreachable size cross-checks. Production - Refuse a zero-length extracted binary, and again at the apply boundary: the checksum covers the archive, not the extracted bytes, so an empty payload verifies against itself. - Check the normalized "//" prefix before path.Clean/path.IsAbs, which collapses UNC paths and made that arm unreachable. Rejection unchanged.
…ilures Close the remaining self-update findings (SFU-10..19). No behaviour change. - syncStagedFileFn seam (test-only): pins that the staged file is fsynced strictly before commit, and that a sync failure aborts before minio renames anything. syncStagedFile's own error propagation is exercised on Unix via a FIFO staged path, since fsync on a FIFO fails; skipped on Windows. - InterruptedUpdate: target present with a leftover .old backup is the documented Windows steady state and must not be reported as interrupted. - newFixtureServerWith(t, opts) adds per-path handler overrides beside the existing newFixtureServer, covering non-200 on both asset downloads, an empty download body, an empty release body and an empty tag_name. - Version parser: the mirrored numeric pre-release comparison, and message assertions that pin the core all-digits guard against strconv.Atoi. "1.+5.3" is documented as NOT reaching that guard: "+" is split off as build metadata first. - verifyChecksum rejects malformed lines with one and with three fields. - extractBinary's unsupported-format arm asserts its own message.
…e tests Adversarial review of PR2+PR3 found four gaps in the extraction guards and their tests, plus doc nits. - extractFromZip filtered only on IsDir(), so a zip entry carrying fs.ModeSymlink named grant.exe was accepted in production and extracted the link-target string as the binary. Not exploitable (extraction is in-memory, the link is never followed, and the bytes are checksum-gated either way), but it was an undocumented tar/zip asymmetry. Now mirrors the tar typeflag guard. - TestExtractBinaryRejectsNonRegularEntries only pinned the error MESSAGE: all its fixtures are header-only tar types, whose bodies Go forces to zero length, so the empty-binary backstop caught them first. Added tar.TypeCont and vendor type 'Z' cases, which have readable bodies and therefore fail on the bytes returned - a behavioral pin for the whole class. - The FIFO fsync test asserted only err != nil, so an open failure would make it pass with the mutation applied. It now asserts EINVAL/ENOTSUP. - Both archive fuzz targets now shrink maxDownloadBytes to 64 KiB. FuzzExtractFromZip previously collapsed to 0 exec/sec while still reporting PASS, because readCapped can io.ReadAll 128 MiB per exec. Docs: CLAUDE.md corrected on the build-exclusion vs skip wording, the type-specific claim, and the backstop's residual gap; mutation ledger updated for SFU-07 and SFU-11 and gains SFU-23.
TestExtractBinaryRejectsNonRegularEntries asserted only the generic "does not contain a grant binary" fallback, which every failure mode produces: neutering isBinaryEntry to return false left the whole table passing. It could not tell the type guard from the name match from checkArchivePath. Each case now pairs the hostile entry with a valid binary under the other accepted name, and asserts that extraction SUCCEEDS and returns that binary. Removing either type guard makes both entries match and the extractor reports "archive contains more than one grant binary"; the valid binary is placed first so the zero-length backstop cannot fire before the duplicate is seen. Neutering isBinaryEntry now fails too. The rejection half of the class is kept as TestExtractBinaryRejectsNonRegularOnlyArchive, explicitly labelled as wording-shaped and carrying no mutation-killing weight of its own. Also records, on the zip oversize-decoy test, that tar's per-entry size check is not an aggregate cap: nothing bounds total inflated bytes or entry count, but verifyChecksum runs before extractBinary, so reaching it requires control of checksums.txt.
The property body called the production hasDriveLetter, so a defect inside it was invisible: narrowing it to uppercase-only drives survived 2.5M execs while TestCheckArchivePath/lowercase_drive caught it instantly. The oracle now uses its own inlined drive-letter predicate and the mutation fails on seed corpus entry #9. Also corrects the doc comment: the target checks a necessary condition on accepted names, not the guard's full contract — an over-eager guard passes it trivially.
The archive-hardening work added eight PR-length paragraphs of test-file structure, fuzz exec-rate diagnostics and mutation-survival rationale to the grant update bullets. Per the repo's own convention that belongs in the PR description; CLAUDE.md keeps policy and architecture. Retained: the load-bearing UNC-before-path.IsAbs ordering, the deliberately symmetric tar/zip non-regular type guards, the intentional declared-size asymmetry, the fact that fuzz targets exist and that a genuine testdata/fuzz failure is committed. Adds one sentence that neither size cap is an aggregate cap, and why that is accepted.
aaearon
force-pushed
the
test/selfupdate-hardening
branch
from
August 16, 2026 07:39
d935efb to
00a6d1a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Parts 2–3 of 8. Base:
test/isolation-harness(#63) — that must merge first.Defensive security work on our own updater. Hostile archive fixtures are built in-memory; nothing is written to disk or followed.
The headline defect
A
tar.TypeSymlinkentry namedgrantwithSize: 0madeextractBinaryreturn(empty, nil).applyBinaryhashes whatever it is handed, so an empty payload verifies against itself and installs — bricking the user's binary. Reproduced twice independently asbytes=0 err=<nil>.Zero-length binaries are now rejected in both extractors and at
applyBinaryTo.Also fixed
grantbeside it plus a guard-specificwantErrContains.path.Clean("//host/share/x")collapses to/host/share/x, sopath.IsAbsalways won. Reordered. Verified message-only: 35 hand-picked inputs plus every string of length ≤5 over{/ \ . : C c 1 a}— 37,449 strings, zero decision differences.fs.ModeSymlinkentry namedgrant.exewas accepted in production, unmutated. Nowf.Mode()&fs.ModeType != 0, mirroring tar.checkArchivePathand both extractors, seeded with the hostile cases.Non-regular entries: pinned behaviorally, not by wording
An earlier revision of this PR claimed each non-regular case had "a valid
grantbeside it plus a guard-specificwantErrContains". That was false, and a review caught it: all seven cases asserted the generic fallback"does not contain a grant binary", and none placed a valid binary beside the hostile entry. NeuteringisBinaryEntryto returnfalseleft the entire table passing, so the test could not distinguish the type guard skipping the entry fromisBinaryEntryfailing to match fromcheckArchivePathrejecting it.TestExtractBinaryRejectsNonRegularEntriesis now inverted into a success assertion. Two entries cannot both be namedgrant, butisBinaryEntryacceptsgrantandgrant.exein either archive format, so each hostile entry is paired with a valid binary under the other name — valid binary first, so the zero-length backstop cannot fire before the duplicate is seen:Mutation evidence (all previously survived or died only on wording):
isBinaryEntry→return falsearchive does not contain a grant binaryhdr.Typeflag != tar.TypeRegarchive contains more than one grant binaryf.Mode()&fs.ModeType != 0→f.FileInfo().IsDir()archive contains more than one grant binary(plus the only-archive case returning 11 bytes)The rejection half of the class is kept as
TestExtractBinaryRejectsNonRegularOnlyArchive, explicitly documented as wording-shaped and carrying no mutation-killing weight of its own.FuzzCheckArchivePath's oracle was not independentThe property body re-implemented
checkArchivePath's switch verbatim — including calling the productionhasDriveLetter— so a defect inside that helper was invisible to the fuzzer. NarrowinghasDriveLetterto uppercase-only drives survived 2,450,979 execs in 30s, whileTestCheckArchivePath/lowercase_drivecaught it instantly.The oracle now uses its own inlined drive-letter predicate. The same mutation is killed on seed corpus entry #9 in 0.12s:
accepted a drive-absolute path: "c:\\grant.exe". The comment claiming the target "asserts the guard's CONTRACT" was also overstated and now says what is true: it checks a necessary condition on accepted names, and an over-eager guard that rejects legitimate names passes it trivially —TestCheckArchivePathowns that side.Notes a reviewer should not skip
tar.Readerforcesnb=0for header-only types (symlink, hardlink, dir, char, block, fifo).TypeContand vendor types'A'..'Z'are not header-only and carry readable bytes — with the type guard removed they would extract attacker-chosen bytes and the zero-length backstop would never fire. This is why the type guard must never be removed in favour of that backstop.zip.NewReaderparses only the central directory and never inflates skipped entries. Measured — a 1000:1 bomb inflated nothing in 126 µs. Pinned with a passing test.hdr.Size > maxDownloadBytesbounds each entry andreadCappedbounds the binary, but nothing bounds total inflated bytes or entry count, andtar.Reader.Next()must inflate every skipped entry to reach the next header. Measured: an 8.3 MB archive walks 65 entries in 2.45s; extrapolated to the 128 MiB cap that is ~40s CPU and ~128 GiB inflated. No code change —verifyChecksumruns beforeextractBinary, so an attacker must already controlchecksums.txt, which the documented trust model already excludes. Recorded in the test comment and in CLAUDE.md.FuzzExtractFromZippreviously wedged to 0 exec/sec and still printedPASS. Diagnosed as throughput collapse on near-cap inputs (128 MiBio.ReadAllper exec), not a production hang. Fixed by cappingmaxDownloadBytesinside the targets; it now sustains the highest throughput of the three (2.95M execs / 30s).len(data) != hdr.Sizecross-checks are unreachable — a successful capped read returns exactlyhdr.Size, and earlier exhaustion returnsio.ErrUnexpectedEOF. Kept as defense-in-depth, claiming no coverage.fsynctest uses a FIFO and is//go:build !windows. No portable Windows equivalent exists —FlushFileBufferssucceeds on both named pipes and regular files.grant updatebullets. Per the repo's own convention that material belongs here, not there; CLAUDE.md now keeps only the UNC ordering, the symmetric type guards, the intentional size-check asymmetry plus the aggregate-cap sentence, and the existence of the fuzz targets.17/17 mutations killed. Adversarial review performed (Codex credits exhausted; review by a Claude agent). All findings fixed, including the two above, which were found in a second adversarial pass against this PR's own claims.